fix: suppress duplicate pending detection SSE broadcasts#2433
Conversation
Only broadcast pending detection snapshots when the snapshot actually differs from the last broadcast (new species, removed species, or increased hit count). Previously every inference cycle broadcast the full snapshot even when nothing changed, spamming SSE clients with identical messages. - Add HitCount field to SSEPendingDetection for change detection - Track last broadcast snapshot and skip identical broadcasts - Update frontend diffPendingSnapshot to detect hitCount increases - Add tests for snapshot change detection and HitCount propagation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the real-time pending detection system by reducing redundant Server-Sent Events (SSE) broadcasts. By only sending updates when meaningful changes occur, it decreases network traffic and improves the efficiency of client-side updates. The changes also enhance the granularity of pending detection data, providing the frontend with more detailed information to react to ongoing inference activity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedPull request was closed or merged during review WalkthroughHit-count tracking was added across the stack: frontend types and overlay logic now handle an optional Changes
Sequence DiagramsequenceDiagram
participant Event as Detection Event
participant Processor as Backend Processor
participant Broadcaster as Broadcast Manager
participant SSE as SSE Stream
participant Frontend as Frontend Client
participant Overlay as Detection Overlay
Event->>Processor: increment pending Count
Processor->>Broadcaster: flushPendingDetections() -> build []SSEPendingDetection{Species, SourceID, HitCount, Status, FirstDetected}
Broadcaster->>Broadcaster: sortPendingSnapshot(curr)
Broadcaster->>Broadcaster: pendingSnapshotChanged(prev, curr)?
alt changed
Broadcaster->>SSE: send SSEPendingDetection[]
SSE->>Frontend: deliver snapshot
Frontend->>Overlay: update pending snapshot
Overlay->>Overlay: diffPendingSnapshot compares hitCount -> mark new activity if increased
else unchanged
Broadcaster-->>Broadcaster: skip send
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of excessive SSE broadcasts by implementing a robust change detection mechanism for pending detection snapshots. The introduction of the hitCount field in both frontend and backend, along with the pendingSnapshotChanged function, ensures that updates are only sent when there's a meaningful change in species composition or activity. The added unit tests comprehensively cover the new logic, including various edge cases for snapshot comparison. The changes are well-implemented and contribute positively to the system's efficiency and maintainability.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| Status: PendingStatusActive, | ||
| FirstDetected: item.CreatedAt.Unix(), | ||
| Source: p.getDisplayNameForSource(item.Source), | ||
| SourceID: item.Source, | ||
| HitCount: item.Count, | ||
| }) | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in e152e50 — extracted sortPendingSnapshot helper and applied it to the flusher's broadcast snapshot before calling broadcastPendingSnapshot.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/analysis/processor/pending_broadcast.go`:
- Around line 155-170: pendingSnapshotChanged does index-based comparisons
assuming both slices are identically ordered, but flushPendingDetections builds
broadcastSnapshot by iterating a map (non-deterministic order), causing false
positives; fix by sorting the broadcastSnapshot slice before calling
broadcastPendingSnapshot in flushPendingDetections (use the same ordering logic
as SnapshotVisiblePending or extract that comparator into a shared sort helper)
so pendingSnapshotChanged compares stable, identically-ordered slices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69c7c89d-3a1d-4dae-b4d9-d4110badf35d
📒 Files selected for processing (6)
frontend/src/lib/types/pending.types.tsfrontend/src/lib/utils/detectionOverlay.test.tsfrontend/src/lib/utils/detectionOverlay.tsinternal/analysis/processor/pending_broadcast.gointernal/analysis/processor/pending_broadcast_test.gointernal/analysis/processor/processor.go
Extract sortPendingSnapshot helper and apply it to the flusher's broadcast snapshot. Map iteration order is non-deterministic in Go, so the unsorted flusher snapshot caused false positives in the index-based pendingSnapshotChanged comparison.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively reduces unnecessary SSE broadcasts by comparing pending detection snapshots and only sending updates when there are actual changes. This is achieved by adding a hitCount to detections and updating both frontend and backend logic to handle it. The changes are well-tested. I've found one potential issue with the sorting logic that could lead to incorrect change detection in edge cases, which aligns with our guidelines on ensuring deterministic ordering, and have provided a suggestion to make it more robust.
| slices.SortFunc(s, func(a, b SSEPendingDetection) int { | ||
| if a.FirstDetected != b.FirstDetected { | ||
| if a.FirstDetected < b.FirstDetected { | ||
| return -1 | ||
| } | ||
| return 1 | ||
| } | ||
| if a.Species < b.Species { | ||
| return -1 | ||
| } | ||
| if a.Species > b.Species { | ||
| return 1 | ||
| } | ||
| return 0 | ||
| }) |
There was a problem hiding this comment.
The current sorting logic in sortPendingSnapshot is not fully deterministic for the comparison performed in pendingSnapshotChanged. The comparison function checks SourceID, but the sort function does not use it as a tie-breaker.
If two detections have the same FirstDetected and Species but different SourceIDs, their relative order is not guaranteed. This can cause pendingSnapshotChanged to incorrectly report a change when the order of items happens to differ between snapshots, even if the set of items is identical.
To ensure a stable comparison, you should also sort by SourceID as a final tie-breaker. I've also taken the liberty to simplify the comparison logic slightly for better readability.
slices.SortFunc(s, func(a, b SSEPendingDetection) int {
if a.FirstDetected < b.FirstDetected {
return -1
}
if a.FirstDetected > b.FirstDetected {
return 1
}
if a.Species < b.Species {
return -1
}
if a.Species > b.Species {
return 1
}
if a.SourceID < b.SourceID {
return -1
}
if a.SourceID > b.SourceID {
return 1
}
return 0
})References
- This aligns with the principle of ensuring deterministic ordering through explicit sorting to prevent non-deterministic behavior, as seen in the rule for thermal zones. Explicitly sorting by all relevant fields, including
SourceIDas a tie-breaker, ensures consistent comparisons and avoids spurious change detections.
There was a problem hiding this comment.
Fixed in latest push — added SourceID as third-level tie-breaker in sortPendingSnapshot.
| } | ||
|
|
||
| // broadcastPendingSnapshot broadcasts a pending detection snapshot via the | ||
| // PendingBroadcaster callback. If no broadcaster is set, this is a no-op. | ||
| // PendingBroadcaster callback only when the snapshot differs from the last | ||
| // broadcast (new species, removed species, or updated hit counts). | ||
| // If no broadcaster is set, this is a no-op. | ||
| func (p *Processor) broadcastPendingSnapshot(snapshot []SSEPendingDetection) { | ||
| p.pendingBroadcasterMu.RLock() | ||
| broadcaster := p.PendingBroadcaster |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed — added SourceID as tie-breaker in sortPendingSnapshot.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/analysis/processor/pending_broadcast.go`:
- Around line 38-56: The current comparator in sortPendingSnapshot (operating on
SSEPendingDetection) only uses FirstDetected and Species, which can leave ties
unresolved when multiple detections share the same Unix timestamp and species
and thus break pendingSnapshotChanged's index-based comparison; update the
comparator in sortPendingSnapshot to produce a total order by adding additional
tie-breakers (at least SourceID, and also include any other stable fields such
as DetectionID or Location if present on SSEPendingDetection) so that every pair
is deterministically ordered before pendingSnapshotChanged performs per-index
diffs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22e3c859-58cc-423f-9069-6a687061718e
📒 Files selected for processing (2)
internal/analysis/processor/pending_broadcast.gointernal/analysis/processor/processor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/analysis/processor/processor.go
pendingSnapshotChanged compares SourceID but the sort didn't include it, causing non-deterministic ordering with multiple sources.
1 participant
Summary
hitCountfield toSSEPendingDetectionso the frontend can detect new inference hits for existing pending speciesdiffPendingSnapshot()to treat increasedhitCountas new activity (not just new species appearing)Fixes https://git.koti/tphakala/birdnet-go/issues/20
Test Plan
TestPendingSnapshotChanged(8 subtests),TestSnapshotVisiblePending_IncludesHitCount,TestBuildFlushNotification_IncludesHitCountdetects increased hitCount as new activity,ignores unchanged hitCount(12 tests total pass)golangci-lint runcleanSummary by CodeRabbit
New Features
Improvements
Tests